Wait for handover state update before serving the requests #7028

yux0 · 2024-12-20T23:57:12Z

What changed?

Wait for handover state update before serving the requests.

Why?

We need to wait on handover state update before the redirect interceptor. It will serve the request once the replication state is updated and routes to the correct endpoint.

How did you test it?

TODO

Potential risks

Documentation

Is hotfix candidate?

yycptt · 2024-12-23T05:44:20Z

common/rpc/interceptor/namespace_validator.go

+		err := ni.checkReplicationState(nsEntry, info.FullMethod)
+		if err != nil {
+			var nsErr error
+			nsEntry, nsErr = ni.namespaceRegistry.RefreshNamespaceById(nsEntry.ID())


hmmm this will keep refreshing the namespace? I mean all requests will perform a refresh upon retry.

also if the handover error is from history service then the refresh here is unnecessary

Make sense. We can also just wait for ns refresh not do this proactive refresh.

common/rpc/interceptor/namespace_validator.go

yycptt · 2024-12-23T05:52:41Z

common/rpc/interceptor/namespace_validator.go

 		return err
 	}
-	return ni.checkReplicationState(namespaceEntry, fullMethod)
+	// Only retry on ErrNamespaceHandover, other errors will be handled by the retry interceptor


shall we just let retry interceptor handle the error? and this interceptor just block until ns is no longer in handover state?

I did not go with this because retry interceptor should be the most inner interceptor. If that is the case, can I add a handover interceptor after the retry interceptor?

I couldn't remember why we moved retry interceptor to be the most inner interceptor, but I think there must be a reason. I think we should fully understand that to make sure the retry logic here won't cause any issue.

can I add a handover interceptor after the retry interceptor?

hmmm then why not just retry handover error in retry interceptor?

I guess I was proposing something different in my comment, i.e. block on ns state change instead of retry (by registering a callback to namespace registry).

I cannot wait for ns callback as the error might return from history nodes and there is no callback from frontend.

common/util.go

yycptt · 2025-01-24T06:42:09Z

common/rpc/interceptor/telemetry.go

@@ -215,10 +215,14 @@ func (ti *TelemetryInterceptor) RecordLatencyMetrics(ctx context.Context, startT
 		userLatencyDuration = time.Duration(val)
 		metrics.ServiceLatencyUserLatency.With(metricsHandler).Record(userLatencyDuration)
 	}
+	handoverRetryLatency := time.Duration(0)
+	if val, ok := metrics.ContextCounterGet(ctx, metrics.NamespaceHandoverRetryLatency.Name()); ok {
+		handoverRetryLatency = time.Duration(val)


so we don't actually emit metric for this latency?

Follow the command to exclude the backoff retry delay from the no user latency.

We need to chat about this.

common/rpc/interceptor/namespace_validator.go

yycptt · 2025-01-24T06:50:09Z

common/rpc/interceptor/namespace_validator.go

+		response, err = handler(ctx, req)
+		if retryCount > 1 {
+			retryLatency = retryLatency + handoverRetryPolicy.ComputeNextDelay(0, retryCount, nil)
+		}
+		retryCount++


it's a bit hard to follow as it's calculating the retryLatency before the current attempt, but the code lives after the handler call...

yycptt · 2025-01-24T06:56:20Z

common/rpc/interceptor/namespace_validator.go

 		return err
 	}
-	return ni.checkReplicationState(namespaceEntry, fullMethod)
+	// Only retry on ErrNamespaceHandover, other errors will be handled by the retry interceptor


I couldn't remember why we moved retry interceptor to be the most inner interceptor, but I think there must be a reason. I think we should fully understand that to make sure the retry logic here won't cause any issue.

can I add a handover interceptor after the retry interceptor?

hmmm then why not just retry handover error in retry interceptor?

I guess I was proposing something different in my comment, i.e. block on ns state change instead of retry (by registering a callback to namespace registry).

yycptt · 2025-02-03T23:19:58Z

common/rpc/interceptor/namespace_handover.go

+	if namespaceData.ReplicationState() == enumspb.REPLICATION_STATE_HANDOVER {
+		cbID := uuid.New()
+		waitReplicationStateUpdate := make(chan struct{})
+		i.namespaceRegistry.RegisterStateChangeCallback(cbID, func(ns *namespace.Namespace, deletedFromDb bool) {


we probably should be change the impl to listen on changes for a specific namespace...
Can be done in a later PR.

common/rpc/interceptor/namespace_handover.go

yycptt · 2025-02-03T23:22:48Z

common/rpc/interceptor/namespace_handover.go

+	if i.enabledForNS(namespaceName.String()) {
+		startTime := i.timeSource.Now()
+		defer func() {
+			metrics.HandoverWaitLatency.With(i.metricsHandler).Record(time.Since(startTime))


I guess we only want to emit the metric when namespace actually waited on the handover state.

yycptt · 2025-02-03T23:24:22Z

service/frontend/fx.go

@@ -267,6 +269,7 @@ func GrpcServerOptionsProvider(
 		namespaceLogInterceptor.Intercept, // TODO: Deprecate this with a outer custom interceptor
 		metrics.NewServerMetricsContextInjectorInterceptor(),
 		authInterceptor.Intercept,
+		namespaceHandoverInterceptor.Intercept,


Can you add some comments explaining why it must be in this place?

Is the handover state checking logic in StateValidationIntercept removed?

I did not remove in this PR as I added the feature flag. Once we can confident we can remove it.

do you want to disabled that the check in StateValidationIntercept if the feature flag is enabled?

yycptt · 2025-02-03T23:28:30Z

service/frontend/fx.go

@@ -267,6 +269,7 @@ func GrpcServerOptionsProvider(
 		namespaceLogInterceptor.Intercept, // TODO: Deprecate this with a outer custom interceptor
 		metrics.NewServerMetricsContextInjectorInterceptor(),
 		authInterceptor.Intercept,
+		namespaceHandoverInterceptor.Intercept,
 		redirectionInterceptor.Intercept,


I guess theoretically we can retry in redirection Interceptor? since it has the knowledge of whether redirect will actually happen or not?

I guess so but the logic could be weird. What should we do if the redirect will not happen?

yycptt · 2025-02-03T23:29:31Z

common/rpc/interceptor/namespace_handover.go

+		case <-waitReplicationStateUpdate:
+		}
+		i.namespaceRegistry.UnregisterStateChangeCallback(cbID)
+		if err != nil {


nit: we don't need this check?

yycptt · 2025-02-27T00:24:07Z

service/frontend/fx.go

@@ -267,6 +269,7 @@ func GrpcServerOptionsProvider(
 		namespaceLogInterceptor.Intercept, // TODO: Deprecate this with a outer custom interceptor
 		metrics.NewServerMetricsContextInjectorInterceptor(),
 		authInterceptor.Intercept,
+		namespaceHandoverInterceptor.Intercept,


do you want to disabled that the check in StateValidationIntercept if the feature flag is enabled?

yycptt · 2025-02-27T00:26:04Z

common/rpc/interceptor/namespace_handover.go

+	methodName string,
+) (waitTime *time.Duration, retErr error) {
+	if _, ok := allowedMethodsDuringHandover[methodName]; ok {
+		return


nit: returning nil, nil is surprising I think for the caller.

yux0 requested a review from a team as a code owner December 20, 2024 23:57

yux0 requested review from yycptt and hai719 December 20, 2024 23:57

yycptt reviewed Dec 23, 2024

View reviewed changes

yycptt reviewed Jan 24, 2025

View reviewed changes

Wait for namespace handover update

26f873f

yux0 force-pushed the handover-err branch from d2e2781 to 26f873f Compare February 3, 2025 22:27

yux0 changed the title ~~Retry handover error during graceful failover~~ Wait for handover state update before serving the requests Feb 3, 2025

yycptt reviewed Feb 3, 2025

View reviewed changes

yux0 added 3 commits February 19, 2025 15:59

clean up code

656d66a

Update metric

f60b2b0

update

557c48c

yycptt approved these changes Feb 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait for handover state update before serving the requests #7028

Wait for handover state update before serving the requests #7028

yux0 commented Dec 20, 2024 •

edited

Loading

yycptt Dec 23, 2024

yux0 Jan 22, 2025

yycptt Dec 23, 2024

yux0 Jan 22, 2025

yycptt Jan 24, 2025

yux0 Jan 27, 2025

yycptt Jan 24, 2025

yux0 Jan 27, 2025

yux0 Jan 27, 2025

yycptt Jan 24, 2025

yycptt Jan 24, 2025

yycptt Feb 3, 2025

yycptt Feb 3, 2025

yycptt Feb 3, 2025

yycptt Feb 3, 2025

yux0 Feb 19, 2025

yycptt Feb 27, 2025

yycptt Feb 3, 2025

yux0 Feb 19, 2025

yycptt Feb 3, 2025

yycptt Feb 27, 2025

yycptt Feb 27, 2025

Wait for handover state update before serving the requests #7028

Are you sure you want to change the base?

Wait for handover state update before serving the requests #7028

Conversation

yux0 commented Dec 20, 2024 • edited Loading

What changed?

Why?

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yux0 commented Dec 20, 2024 •

edited

Loading